Adjust WithExtraQueryParameters APIs and cache key behavior #5536
+626
−59
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… deprecate existing WithExtraQueryParameters APIs
bgavrilMS
reviewed
Oct 20, 2025
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
bgavrilMS
reviewed
Oct 20, 2025
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
bgavrilMS
requested changes
Oct 20, 2025
…uireTokenParameterBuilder.cs Co-authored-by: Bogdan Gavril <[email protected]>
bgavrilMS
reviewed
Oct 22, 2025
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
trwalke
reviewed
Oct 24, 2025
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where extra query parameters could affect token validity but were not included in cache keys, potentially causing invalid cached tokens to be returned when parameters changed between requests.
- Introduces a new
WithExtraQueryParametersAPI acceptingIDictionary<string, (string value, bool includeInCacheKey)>for granular cache key control - Deprecates existing
WithExtraQueryParametersmethods that accept string orIDictionary<string, string>parameters - Updates test code and dev apps to use the new API or suppress deprecation warnings
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/devapps/NetFxConsoleTestApp/Program.cs | Updated to use new tuple-based API |
| tests/devapps/NetCoreTestApp/Program.cs | Updated to use new tuple-based API |
| tests/devapps/DesktopTestApp/PublicClientHandler.cs | Added deprecation warning suppressions for old API usage |
| tests/Microsoft.Identity.Test.Unit/TelemetryTests/OTelInstrumentationTests.cs | Updated tests to use new tuple-based API |
| tests/Microsoft.Identity.Test.Unit/TelemetryTests/HttpTelemetryTests.cs | Updated tests to use new tuple-based API |
| tests/Microsoft.Identity.Test.Unit/RequestsTests/IntegratedWindowsAuthUsernamePasswordTests.cs | Updated to use new test constant |
| tests/Microsoft.Identity.Test.Unit/PublicApiTests/ExtraQueryParametersTests.cs | New test class demonstrating cache key behavior |
| tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs | Updated to use new test constant |
| tests/Microsoft.Identity.Test.Unit/PublicApiTests/CacheKeyExtensionTests.cs | Added tests for API interaction |
| tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs | Updated to use new tuple-based API |
| tests/Microsoft.Identity.Test.Unit/OAuthClientTests.cs | Updated to use new tuple-based API |
| tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AcquireTokenInteractiveBuilderTests.cs | Updated to use new tuple-based API |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/FmiIntegrationTests.cs | Updated integration tests to use new API |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/Agentic.cs | Updated to use new tuple-based API |
| tests/Microsoft.Identity.Test.Common/TestConstants.cs | Added new test constant for tuple-based parameters |
| src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs | Added helper method to convert old dictionary format to new tuple format |
| src/client/Microsoft.Identity.Client/PublicApi/*/PublicAPI.Unshipped.txt | Documents new public API methods |
| src/client/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj | Added System.ValueTuple package reference |
| src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs | Implemented new API and deprecated old ones |
| src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs | Implemented new API and deprecated old ones |
| src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs | Deprecated old string-based API |
tests/Microsoft.Identity.Test.Unit/PublicApiTests/ExtraQueryParametersTests.cs
Outdated
Show resolved
Hide resolved
…rametersTests.cs Co-authored-by: Copilot <[email protected]>
…b.com/AzureAD/microsoft-authentication-library-for-dotnet into avdunn/extra-query-params-api-changes
bgavrilMS
reviewed
Nov 3, 2025
| // Helper method intended to help deprecate some WithExtraQueryParameters APIs. | ||
| // Convert from Dictionary<string, string> to Dictionary<string, (string value, bool includeInCacheKey)>, | ||
| // with all includeInCacheKey set to false by default to maintain existing behavior of those older APIs. | ||
| internal static IDictionary<string, (string value, bool includeInCacheKey)> ConvertToTupleParameters(IDictionary<string, string> parameters) |
There was a problem hiding this comment.
nit: It's not really a "core helper" and could be included in ApiConfig or AppConfig
btw, you can rewrite this in 1-liner
return (parameters ?? Enumerable.Empty<KeyValuePair<string, string>>())
.ToDictionary(
kvp => kvp.Key,
kvp => (kvp.Value, false),
StringComparer.OrdinalIgnoreCase);
bgavrilMS
approved these changes
Nov 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is for KR 3310905 and issue #5361.
In short, there is a problem in our existing
WithExtraQueryParametersbehavior: the extra query parameters could affect tokens but were not used as part of the cache keys, so if theWithExtraQueryParametersvalues changed between requests we could return cached tokens that were not valid for the new request. However, simply adding the extra query parameters to the cache key would not work: it would break existing cache lookup behavior, and not all parameters should be cached.This PR tries to solve that problem with the following changes:
WithExtraQueryParametersthat takes in aIDictionary<string, (string value, bool includeInCacheKey)>CacheKeyComponentsfield, which already does the task of adding extra parameters to the cache keyBaseAbstractAcquireTokenParameterBuilder,AbstractApplicationBuilderWithExtraQueryParametersAPIs, and have them call the new API to set theExtraQueryParametersfieldsAbstractAcquireTokenParameterBuilder,BaseAbstractAcquireTokenParameterBuilder,AbstractApplicationBuilderCoreHelpersto covert the old Dictionary style to the new oneThis PR also adds some new tests to cover the new behavior, and makes some small changes to existing tests:
ExtraQueryParametersTests: A new test class with tests covering the new cache key behavior, as well as demonstrating the behavior of the deprecatedWithExtraQueryParametersAPIsCacheKeyExtensionTests: A new test to show the newWithExtraQueryParametersAPI does not conflict with the existingWithAdditionalCacheKeyComponentsAPI from the extensibility package